-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Filter Development #68
base: master
Are you sure you want to change the base?
Conversation
Initial commit for filter-dev branch. Introduces an abstract base class for support of filter implementation, as well as a basic, first version of a low-pass filter child class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you're on the right track! One major structural change, but otherwise just some documentation/formatting updates. Looking forward to seeing the full implementation!
TODOs
- Make abstract filter and the provided child classes type-agnostic (at least for POD scalars, maybe also for scalar containers).
- Make abstract
Filter
class into a type-agnostic class template. - Make implemented
LowPassFilter
class into a type-agnostic class template.
- Make abstract
- Fix up the formatting per the style guide. Here are some general rules to note. I recommend you get an auto-formatter set up to use the default
clang-format
Google
style (see this page for information on setting this up in VSCode).- All code should use two-space indents, and have a hard break at 80 characters (with a few, select exceptions). The only exception to the two-space intents are modifications to existing code which already uses four-space indents.
- Opening brackets should follow on the same line as a function/object/namespace declaration.
- Namespace contents should not be indented, per this section.
- Add documentation comments to all classes, members, and functions. Be sure to use Doxygen-friendly comments or comment blocks.
Resources
I recommend you reference the following pages as you write code.
- The Google C++ Style Guide for Drake is the style guide for all C++ code in this repository. All naming, formatting, and general structure should follow this.
- The Doxygen Manual outlines how to comment your code so that the documentation generates correctly.
- The ISO C++ Core Guidelines provide all kinds of guidelines for writing C++ code. I link to a few of these in my comments, but here is the full page if you want to check it out.
namespace kodlab | ||
{ | ||
|
||
class Filter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[General] Filter
should be a class template. At a minimum, it should be assumed that Filter
takes in some kind of scalar type (e.g., int
, float
, etc.). Abstracting further, it could take in containers containing scalars (e.g., Eigen::Vector<float, ...>
, std::array<double, ...>
, std::vector<int>
, etc.). Your current implementation only accepts Eigen::Vector3f
data, which is not always the desired type.
For the scalar case, this might be
class Filter { | |
template<typename Scalar> | |
class Filter { |
Then, the data types inside would be Scalar
instead of Eigen::Vector3f
.
namespace kodlab | ||
{ | ||
|
||
class LowPassFilter : public Filter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[General] Same comment as in Filter
. This should probably be a class template.
Note that, for this class, both the definition and implementation would have to live in the header file if you make this a class template. See here for more information.
LowPassFilter::LowPassFilter(float dt, float k_frequency_cutoff, Eigen::Vector3f raw_data) { | ||
|
||
dt_ = dt; | ||
raw_data_ = raw_data; | ||
filtered_data_ = raw_data; | ||
k_frequency_cutoff_ = k_frequency_cutoff; | ||
|
||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[General] Per this ISO C++ Standard item, you should use an initializer list here instead of assignment.
Introduces an abstract
Filter
class from which discrete-time filter implementations can be derived. Also provides severalFilter
implementation classes corresponding to various commonly used filter types.This is a new feature. Addresses #62.
Change List
Filter
object, an abstract class for implementing discrete-time filtering.LowPassFilter
object (a child ofFilter
), which implements a discrete time low-pass filter.Upcoming Changes
DifferenceEquation
object (name subject to change) which will determine a LCCDE from Transfer Function polynomials.DifferenceEquation
, including:HighPassFilter
object implementing a high-pass filterBandPassFilter
object implementing a band-pass filterBandStopFilter
object implementing a band-stop filterNotchFilter
object implementing a notch filterPotential (Long-Term) Future Changes
Testing
This PR draft has yet to be tested. Testing will be conducted by implementing the various filter implementations in the
SimpleControlIOExample
example behavior.Questions
Any feedback on how to improve or enhance the generalizability of this feature would be much appreciated.